-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: ips action test cases #1952
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,5 @@ | |||
pass udp any any -> any 6081 (sid:1;) | |||
pass tcp 2a03:b0c0:0002:00d0:0000:0000:0bd3:4001 any -> 2606:2800:0220:0001:0248:1893:25c8:1946 443 (msg:"PASS_CUSTOM_RULE TCP port:443 to support traffic"; flow:established; sid:201000044;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not apply the pass
to the flow, but should it? The flow starts as not_established
, but after transitioning to established
it stays that way, even if TCP state moves to "closed".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would indeed be coherent with the rule language as we read it.
match: | ||
event_type: tls | ||
- filter: | ||
min-version: 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavior change between 6 and 7+master. I think it makes sense, as the app_proto can change.
args: | ||
- --set stream.midstream=false | ||
- -k none | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it important to run these as IPS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are, because ips is in the test name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, if the test says ips-state SV will run them as IPS? 🤯
Is this PR a draft to discuss current vs expected behavior ? |
pass tcp 2a03:b0c0:0002:00d0:0000:0000:0bd3:4001 any <> 2606:2800:0220:0001:0248:1893:25c8:1946 443 (msg:"PASS_CUSTOM_RULE TCP port:443 to support traffic"; flow:established; sid:201000044;) | ||
|
||
pass tcp 2a03:b0c0:0002:00d0::/64 any <> any [[80,443]] (msg:"PASS_HTTP_NOT_ESTABLISHED TCP allow http/https traffic to the established state to allow further inspection"; flow:not_established; sid:201000012;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to support better seeing what's going on, couldn't we add alert
to these pass rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, if we do, enable verdict
for the alert events, too?
count: 0 | ||
match: | ||
event_type: flow | ||
flow.action: "drop" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, just to see if we agree on this: shouldn't we see flow.action
in the logs, here? I don't see it, at all.
Should this PR be a draft ? |
I think the most interesting thing here is the difference between 6 and 7/master for the "b" case.
Also, we can debate if "a-01" should apply the pass to the flow. I'll put an inline comment too.